Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #10846: [Squirrel] Ensure sqvector size does not overflow #10848

Merged
merged 1 commit into from
May 20, 2023

Conversation

glx22
Copy link
Contributor

@glx22 glx22 commented May 20, 2023

Fixes #10846

Motivation / Problem

array(4611686018427387904) implies allocating 4611686018427387904 * 16 (size of SQObjectPtr) and that is 0 in 64bit world.
And as there is no check for overflow we happily allocate 0 bytes of memory and use it like if we allocated the expected number of elements.

Description

Check for overflow of newsize * sizeof(T) when reallocating sqvector.
As sqvector is deep inside squirrel we don't have access to the VM and the only way to signal the error is throwing an exception. The only catchable exception without adding special handling inside squirrel itself, SQVM catches ... and rethrows, is Script_FatalError so I used it.

Adding overflow check in https://github.com/OpenTTD/OpenTTD/blob/master/src/3rdparty/squirrel/squirrel/sqbaselib.cpp#L217 could work for array() but would not protect when resizing the array after its creation, like in

local x = array(10);
x.resize(4611686018427387904);

Limitations

Squirrel compiler also uses sqvector but it only catches SQChar *, so throwing something else might be an issue. Anyway compiler is less likely to try to allocate enough to trigger an overflow.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@glx22 glx22 enabled auto-merge (squash) May 20, 2023 14:07
@glx22 glx22 merged commit 8d2a0a7 into OpenTTD:master May 20, 2023
19 checks passed
@glx22 glx22 deleted the fix_10846 branch May 20, 2023 14:45
@rubidium42 rubidium42 added the backport requested This PR should be backport to current release (RC / stable) label May 31, 2023
@glx22 glx22 added backported This PR is backported to a current release (RC / stable) and removed backport requested This PR should be backport to current release (RC / stable) labels Jun 4, 2023
mrmbernardi pushed a commit to mrmbernardi/OpenTTD that referenced this pull request Jul 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported This PR is backported to a current release (RC / stable)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Crash]: script executing array(2**62) overflows allocated memory counter and crashes the game
2 participants